Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce JEP for External Fingerprint Storage #289

Merged
merged 12 commits into from
Jun 12, 2020

Conversation

stellargo
Copy link
Contributor

@stellargo stellargo requested a review from a team as a code owner June 10, 2020 18:29
Copy link
Member

@oleg-nenashev oleg-nenashev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good as a JEP draft, thanks! The format is followed.

I will merge it in 24 hours if there is no negative feedback at the Cloud Native SIG meeting

jep/0000/README.adoc Outdated Show resolved Hide resolved
jep/0000/README.adoc Outdated Show resolved Hide resolved
jep/0000/README.adoc Outdated Show resolved Hide resolved
jep/0000/README.adoc Outdated Show resolved Hide resolved
Copy link

@afalko afalko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comments; mostly small legibility updates


When a plugin is configured and enabled, the fingerprints are transferred from the local storage to the external storage.

This by followed by clearing the local database of fingerprints from Jenkins HOME ONLY if the writes are successful.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is followed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @afalko, thanks for reviewing. No this is not followed yet, but according to JEP's guidelines, we need to frame it in the present tense as if it is already done [source].

jep/0000/README.adoc Outdated Show resolved Hide resolved

We do not propose to revamp the `FingerprintCleanupThread` to use the above logic at the moment.

==== External to Local Migration (Nice 2 Have)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change "nice 2 have"s to "out of scope for MVP" ... or at the very least, s/2/to/ ;)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd probably actually move these sections to the bottom so they don't distract from the important stuff.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@afalko thanks! Incorporated "out of scope for MVP" :)
I have these at the end of the Design Decisions section

jep/0000/README.adoc Outdated Show resolved Hide resolved
@oleg-nenashev
Copy link
Member

oleg-nenashev commented Jun 12, 2020 via email

Copy link
Member

@jglick jglick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Structure of JEP looks good to me and writing is clear. The (out of scope for MVP) sections could perhaps be moved to Reasoning (i.e., why they are out of scope), but I think it is also OK under Specification as you are just noting in which phase something might be implemented.

jep/0000/README.adoc Outdated Show resolved Hide resolved
Co-authored-by: Jesse Glick <[email protected]>
@oleg-nenashev
Copy link
Member

JEP-225 ID is locked by #266. It does not really follow the JEP process, but I do not want to provoke additional merge conflicts. We are not required to do sequential numbering. I will merge one as JEP-226.

@oleg-nenashev oleg-nenashev merged commit e9b2a5c into jenkinsci:master Jun 12, 2020
@@ -1,4 +1,4 @@
= JEP-0000: External Fingerprint Storage
= JEP-226: External Fingerprint Storage
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for pointing it out @jglick, Ill submit a PR to update it

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Somebody will do it, if course.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants